Skip to content

stupid fix for coercion hack perf regression #139171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Mar 31, 2025

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 31, 2025
@lcnr
Copy link
Contributor Author

lcnr commented Mar 31, 2025

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 31, 2025
@bors
Copy link
Collaborator

bors commented Mar 31, 2025

⌛ Trying commit e57808d with merge b6303fd...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2025
stupid fix for coercion hack perf regression

cc rust-lang#136127 rust-lang#138542

r? `@ghost`
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 31, 2025
more interesting fix for coercion hack perf regression

cc rust-lang#136127 rust-lang#138542 rust-lang#139171

r? `@ghost`
@bors
Copy link
Collaborator

bors commented Mar 31, 2025

☀️ Try build successful - checks-actions
Build commit: b6303fd (b6303fdafa669e76fca4cabfa942c07295d3f457)

@rust-timer

This comment has been minimized.

@lcnr
Copy link
Contributor Author

lcnr commented Apr 1, 2025

@rust-timer build b6303fd

1 similar comment
@compiler-errors
Copy link
Member

@rust-timer build b6303fd

@compiler-errors
Copy link
Member

Oh, this is the perf results: https://perf.rust-lang.org/compare.html?start=10a76d634781180b4f5402c519f0c237d3be6ee6&end=b6303fdafa669e76fca4cabfa942c07295d3f457

That's why rust-timer isn't requeueing the job, b/c it's already done

@lcnr
Copy link
Contributor Author

lcnr commented Apr 1, 2025

it seems like the perf results are pretty much equivalent between this and #138542. Would prefer going ahead with this PR @compiler-errors

it has a better vibe for me. I think the big part is avoiding to use may_coerce in the happy path and keeping casts as

  • try coerce
  • if that fails, cast

@compiler-errors
Copy link
Member

I'm still mostly in favor of the approach to relocate a (solely) cast-related hack to casting code, which is the primary motivation of #138542.

I know that you seem to have a negative opinion of the added complexity of the casting code in that PR, but IMO that added complexity (which is now bubbled up into check_cast) reflects the complexity of the change being introduced in #138542 and its interaction with the strangely (possibly load bearing) false-positive-biased behavior of coerce_unsized.

There's quite a few usages of coercion around HIR typeck, and none of them care about *const dyn Tr -> *const W<dyn Tr>-like coercions other than as casts, so I don't see why we wouldn't want to localize this behavior as closely to the code that concerns it to avoid its behavioral side-effects even needing to be considered for other types of coercion.

Making coerce_unsized less complicated is definitely a benefit that this PR doesn't really have, even if it recovers most of the perf loss in the original PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-perf Status: Waiting on a perf run to be completed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants